Upgrade test fixtures to 2.x project layouts #1301

raymondfeng · 2015-04-14T21:37:30Z

bajtos · 2015-04-15T09:07:11Z

common/models/user.json

+      "foreignKey": "userId",
+      "options": {
+        "disableInclude": true
+      }


FWIW, this option is not covered by your new tests, because the tests are explicitly overriding this option in models.json. Please consider writing few more test to verify that a) the built-in user disables include of access tokens b) user submodels inherit this setting by default too.

bajtos · 2015-04-15T09:08:09Z

The model change is not covered by tests. Other than that, the patch LGTM.

raymondfeng · 2015-04-17T18:40:19Z

@bajtos PTAL again. I had to convert the test apps into LB 2.x layout, which is a great side effect.

bajtos · 2015-04-20T08:23:55Z

I had to convert the test apps into LB 2.x layout, which is a great side effect.

For posterity, can you please explain why was this needed?

LB 2.x project layout does not use defaultForType option, please remove it from all datasources.json files

bajtos · 2015-04-20T08:26:16Z

test/fixtures/user-integration-app/server/server.js

+app.use(apiPath, loopback.rest());
+app.use(loopback.static(path.join(__dirname, 'public')));
+app.use(loopback.urlNotFound());
+app.use(loopback.errorHandler());


Is there any particular reason why you are not using middleware.json and middleware phases in this app?

Are your tests relying on cookieParser at all? And why do they need static files from the public directory?

No, I just copied them from the existing fixture. I can clean them up.

bajtos · 2015-04-20T08:38:37Z

I had to convert the test apps into LB 2.x layout, which is a great side effect.

Actually I think this is a wrong thing to do. LoopBack tests should not depend on loopback-boot, they should setup the application via code only.

Loading an external app from fixtures makes tests difficult to understand, one has to scan many files to understand what's going on.
loopback-boot is an optional component providing opinionated convention for project layout. LoopBack must work without loopback-boot too, and thus the unit-tests should not depend on loopback-boot.

IIRC, the main reason why the existing LoopBack tests are depending on loopback-boot is to ensure that LoopBack 1.x apps can be upgraded to LoopBack 2.x + loopback-boot 1.x with no breaking changes.

I think this issue deserves some discussion and it will be best to decouple it from your work on "Set disableInclude for User->AccessToken relation". Is there any way how to finish your fix without upgrading loopback-boot?

raymondfeng · 2015-04-20T15:33:13Z

Actually I think this is a wrong thing to do. LoopBack tests should not depend on loopback-boot, they should setup the application via code only.
Loading an external app from fixtures makes tests difficult to understand, one has to scan many files to understand what's going on.
loopback-boot is an optional component providing opinionated convention for project layout. LoopBack must work without loopback-boot too, and thus the unit-tests should not depend on loopback-boot.
IIRC, the main reason why the existing LoopBack tests are depending on loopback-boot is to ensure that LoopBack 1.x apps can be upgraded to LoopBack 2.x + loopback-boot 1.x with no breaking changes.
I think this issue deserves some discussion and it will be best to decouple it from your work on "Set disableInclude for User->AccessToken relation". Is there any way how to finish your fix without upgrading loopback-boot?

The project already has dependency on loopback-boot before the PR. I simply upgrade the fixtures to 2.x layout.

Migrating from 1.x to 2.x is not a main concern any more. I think it's better to use 2.x layout as developers sometimes try to follow what we do. If you feel strongly about removing the test fixtures, we can discuss that as a separate issue.

With 1.x layout, there is no way to use the base class such as User for REST. The test is meant to cover our main stream - LB 2.x.

bajtos · 2015-04-20T16:02:15Z

The project already has dependency on loopback-boot before the PR. I simply upgrade the fixtures to 2.x layout.

Migrating from 1.x to 2.x is not a main concern any more. I think it's better to use 2.x layout as developers sometimes try to follow what we do. If you feel strongly about removing the test fixtures, we can discuss that as a separate issue.

With 1.x layout, there is no way to use the base class such as User for REST. The test is meant to cover our main stream - LB 2.x.

I dig up the commit that added the dependency on loopback-boot - see c896c78. It seems the change was made purely in order to support test fixtures booting the app via loopback.boot(), in which case I agree with you that it's ok to upgrade loopback-boot to 2.x.

However, I still believe that we should keep the number of fixtures as small as possible, create loopback apps inside the tests purely code-wise and keep these apps as small as possibly.

raymondfeng · 2015-04-20T16:12:11Z

I decoupled the LB 2.x upgrade from disableInclude User.accessTokens. This PR is for the upgrade.

bajtos · 2015-04-20T16:17:32Z

test/user.test.js

+    // to the wrong app instance
+    defaultApp = loopback.User.app;
+    loopback.User.app = null;
+    User = loopback.User.extend('TestUser', {}, {http: {path: 'test-users'}});


Is there any particular reason why your are not using HTTP path /users? That way you will not have to change URLs in all the test cases below.

Yes, because the model in use is a sub-model of User. If we don't explicitly specify http.path, it will default to /testusers.

So why can't you specify http.path: 'users', or even plural: 'Users', since you are already overriding the default?

Anyways. If you believe that it's better to have /test-users URL even at the cost of making future code archaeology more difficult, then I can live with that.

bajtos · 2015-04-20T16:18:53Z

test/user.test.js

@@ -78,7 +88,7 @@ describe('User', function() {
        assert(err);
        assert.equal(err.name, 'ValidationError');
        assert.equal(err.statusCode, 422);
-        assert.equal(err.details.context, 'user');
+        assert.equal(err.details.context, 'TestUser');


The following is better as it should be resistant against future changes:

assert.equal(err.details.context, TestUser.modelName);

Sure. Fixed.

bajtos · 2015-04-20T17:15:36Z

LGTM.

…ss-tokens Upgrade test fixtures to 2.x project layouts

raymondfeng added the #review label Apr 14, 2015

raymondfeng assigned bajtos Apr 14, 2015

raymondfeng force-pushed the feature/disable-include-access-tokens branch from 5ce2eb6 to 7349a80 Compare April 14, 2015 22:35

bajtos reviewed Apr 15, 2015
View reviewed changes

bajtos assigned raymondfeng and unassigned bajtos Apr 15, 2015

raymondfeng force-pushed the feature/disable-include-access-tokens branch 4 times, most recently from 9abcb08 to 68a2344 Compare April 17, 2015 18:39

raymondfeng assigned bajtos and unassigned raymondfeng Apr 17, 2015

bajtos reviewed Apr 20, 2015
View reviewed changes

bajtos assigned raymondfeng and unassigned bajtos Apr 20, 2015

raymondfeng force-pushed the feature/disable-include-access-tokens branch from 68a2344 to bb3f971 Compare April 20, 2015 15:51

raymondfeng force-pushed the feature/disable-include-access-tokens branch from bb3f971 to b6efccc Compare April 20, 2015 16:08

raymondfeng changed the title ~~Set disableInclude for User->AccessToken relation~~ Upgrade test fixtures to 2.x project layouts Apr 20, 2015

bajtos reviewed Apr 20, 2015
View reviewed changes

Upgrade test fixtures to use LB 2.x layout

12e19e3

raymondfeng force-pushed the feature/disable-include-access-tokens branch from b6efccc to 12e19e3 Compare April 20, 2015 16:23

raymondfeng added a commit that referenced this pull request Apr 20, 2015

Merge pull request #1301 from strongloop/feature/disable-include-acce…

ca004ad

…ss-tokens Upgrade test fixtures to 2.x project layouts

raymondfeng merged commit ca004ad into master Apr 20, 2015

raymondfeng removed the #review label Apr 20, 2015

Upgrade test fixtures to 2.x project layouts #1301

Upgrade test fixtures to 2.x project layouts #1301

Uh oh!

Conversation

raymondfeng commented Apr 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Apr 15, 2015

Uh oh!

raymondfeng commented Apr 17, 2015

Uh oh!

bajtos commented Apr 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Apr 20, 2015

Uh oh!

raymondfeng commented Apr 20, 2015

Uh oh!

bajtos commented Apr 20, 2015

Uh oh!

raymondfeng commented Apr 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Apr 20, 2015

Uh oh!

Uh oh!